-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add support for tokens in toml files #3047
feat: Add support for tokens in toml files #3047
Conversation
Thanks for the pull request, @xitij2000! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
✅ Deploy Preview for paragon-openedx ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
331d991
to
f325bb3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and LGTM
@adamstankiewicz Could we get your thoughts on this? |
@bradenmacdonald I have added this to the agenda for tomorrow's Paragon WG meeting. |
We discussed this in the working group meeting last week and the benefits of toml are absolutely worth merging this for!
This lines up perfectly with what was discussed in the working group - in the meeting notes we noted:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for this! Sorry it took so long to discuss/review it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@xitij2000 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
🎉 This PR is included in version 23.0.0-alpha.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Thanks for looking into this and merging it! |
Description
Currently, the only way to specify tokens is using JSON, however there are some unique benefits to using the TOML format for design tokens. TOML is a format for configuration files that resembles the
ini
but can support richer nested data. It has been embraced as the default format by Python forpyproject
files and by Rust for it'sCargo.toml
files.There are a couple of reasons why it's particularly good for the usecase of design tokens.
Design tokens are deeply nested and in JSON it can get a bit clumsy to represent this. To take a particularly deeply nested value, we have
other.content.form.control.checkbox.indicator.icon-checked.base
which is 8 levels deep. The JSON for such deeply nested values can be pretty verbose and can be hard to follow. In comparison with TOML this is just:The above dot notiation very easily maps to the final variable name, and you can quickly do a search for such nested data without checking the full heirarchy.
In the above example, if you wanted to add value for
other.content.form
you can simply do that at any pointin the document. Unlike JSON where you'd need to have it nested in the correct hierarchy.
If in the above example, you wanted to change from the
checkbox.indicator
tocheckbox-indicator
or move fromother.content.form
toother.form
only for this value without affecting other values, you can do that with just a single change without impacting any otherpart of the hierarchy. With JSON if you rename an attribute that automatically affects all the values in that heirarchy, so you'd have to move this bit of the document to a new attribute. TOML really simplifies refactoring.While design tokens can support comments via field, this is part of the document and will be processed as such. This is not a huge issue, but still a nice to have.
NOTE: This PR merely adds a way to support TOML for design tokens. It is not a recommendation to switch the entirety of the token system to use toml instead of JSON.
While doing some work on design tokens I had to deal with a lot of cases where I'd expressed the hierarchy of values incorrectly and it was a pain to make the changes. I found this much easier with toml so began converting JSON to TOML to make the changes and then convert back. I figured it would be nice to simply add support to Paragon itself since it was a small enough change.
Testing
You can test this by creating a minimal setup for design tokens.
light
folder create atoml
file with some design tokens, for example:./bin/paragon-scripts.js build-tokens --source /tmp/toml-tokens/ --build-dir /tmp/toml-tokens/build --source-tokens-only
build
directory in the folder and you'll see a file calledthemes/light/variables.css
that has a css variable called--pgn-color-testing
with the above value.Deploy Preview
NA
Merge Checklist
example
app?Post-merge Checklist